feat(core/date-input): date input validation improvements#2543
feat(core/date-input): date input validation improvements#2543RamVinayMandal wants to merge 54 commits into
Conversation
… state Co-authored-by: Copilot <copilot@github.com>
🦋 Changeset detectedLatest commit: 1d16592 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a clear() method to the ix-date-input component and refines its validation logic to better handle novalidate forms and programmatic value resets. The changes include a new utility for clearing inputs and extensive regression tests. Review feedback suggests that date parsing should still be performed even when visual validation is suppressed to avoid invalid internal states. Furthermore, the reviewer recommends explicitly synchronizing validation classes during programmatic clears and ensuring that asynchronous validation calls are correctly awaited in property watchers.
… validation Co-authored-by: Copilot <copilot@github.com>
…e-is-empty-for-date
Co-authored-by: Copilot <copilot@github.com>
…x dates Co-authored-by: Copilot <copilot@github.com>
…e-is-empty-for-date
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a clear() method to the ix-date-input component and refactors its validation logic to correctly handle novalidate forms and programmatic value resets. The changes include adding a syncValidationClasses method, updating the value watcher, and providing comprehensive regression tests. Feedback focuses on ensuring that asynchronous validation methods are properly awaited, stale validation classes are removed when validation is suppressed (e.g., in novalidate forms), and that accessibility coverage is maintained with an axe-based component test as per the repository style guide.
…hen-value-is-empty-for-date
- Introduced a new `reportValidity` method to explicitly trigger validation and surface errors immediately, regardless of user interaction. - Updated validation logic to ensure required fields show appropriate error messages when empty. - Improved internal state management for invalid inputs, ensuring visual feedback aligns with user interactions. - Refactored validation classes and error messaging to provide clearer user feedback, including handling of required fields and parse errors. - Added tests to cover new validation scenarios, including behavior in `novalidate` forms and programmatic value changes.
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…sistency - Moved and restructured tests related to initial invalid values and user interactions. - Consolidated required and optional field behavior tests for better readability. - Enhanced test descriptions for improved understanding of scenarios. - Ensured consistent handling of visual validation states across tests.
…e-is-empty-for-date # Conflicts: # packages/core/src/components/date-input/date-input.tsx # packages/react/src/components.server.ts # packages/vue/src/components.ts
…add accessibility tests
…e-is-empty-for-date
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant validation improvements to the ix-date-input component, including new clear() and reportValidity() methods, a customizable i18nErrorRequired property, and proper validation suppression within novalidate forms. The feedback highlights a potential runtime crash in Luxon when minDate or maxDate is undefined, suggesting safe guards in getDateValidation. Additionally, the reviewer notes that when enableTopLayer is active, focus transitions to the dropdown are incorrectly treated as external, and recommends passing the dropdown element reference to handlePickerInputBlur and handlePickerHostFocusout to prevent premature validation and closure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ve validation handling
…d keyboard navigation tests
…e-is-empty-for-date
…on to prevent error flash on required empty fields
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
1-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd issue linkage in the changeset summary (
Fixes #<issue-number>).The summary is missing the required issue-closing reference for release automation.
Suggested fix
Added `i18nErrorRequired` prop (`i18n-error-required`) to customize the required-field error message. + +Fixes #<issue-number>As per coding guidelines, "Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/feat-date-input-validation.md around lines 1 - 20, The changeset file is missing the required issue-closing reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary section (after the package and version type declaration and before or within the description) to enable automatic GitHub issue closure when the changeset is released. Replace <issue-number> with the actual GitHub issue number that this changeset addresses.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/date-input/date-input.tsx`:
- Around line 795-804: The onFocusout event handler is closing the dropdown
whenever focus moves to any element outside the input field, including internal
component elements like the picker. Instead of only checking if relatedTarget is
not null, add an additional check to verify that the relatedTarget is outside
the component itself by using contains() to check if this.el (the component's
root element) contains the relatedTarget. Only call this.closeDropdown() when
focus actually moves outside the component boundaries, not just when moving
between internal child elements like the input and picker.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 397-404: After the mount(...) call that creates the ix-date-input
component, add a hydration assertion before any interactions occur. Insert an
await expect statement that checks if the dateInput locator has the hydrated
class using the regex pattern /\bhydrated\b/. This assertion should be placed
immediately after the dateInput variable assignment and before the click() call
to open-calendar, ensuring the component is fully hydrated before attempting to
interact with it and avoiding potential timing flakes.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 1-20: The changeset file is missing the required issue-closing
reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary
section (after the package and version type declaration and before or within the
description) to enable automatic GitHub issue closure when the changeset is
released. Replace <issue-number> with the actual GitHub issue number that this
changeset addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6635ad04-c22a-4c03-bb89-472d07f83ed1
📒 Files selected for processing (4)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
1-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd issue linkage in the changeset summary (
Fixes #<issue-number>).The summary is missing the required issue-closing reference for release automation.
Suggested fix
Added `i18nErrorRequired` prop (`i18n-error-required`) to customize the required-field error message. + +Fixes #<issue-number>As per coding guidelines, "Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/feat-date-input-validation.md around lines 1 - 20, The changeset file is missing the required issue-closing reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary section (after the package and version type declaration and before or within the description) to enable automatic GitHub issue closure when the changeset is released. Replace <issue-number> with the actual GitHub issue number that this changeset addresses.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/date-input/date-input.tsx`:
- Around line 795-804: The onFocusout event handler is closing the dropdown
whenever focus moves to any element outside the input field, including internal
component elements like the picker. Instead of only checking if relatedTarget is
not null, add an additional check to verify that the relatedTarget is outside
the component itself by using contains() to check if this.el (the component's
root element) contains the relatedTarget. Only call this.closeDropdown() when
focus actually moves outside the component boundaries, not just when moving
between internal child elements like the input and picker.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 397-404: After the mount(...) call that creates the ix-date-input
component, add a hydration assertion before any interactions occur. Insert an
await expect statement that checks if the dateInput locator has the hydrated
class using the regex pattern /\bhydrated\b/. This assertion should be placed
immediately after the dateInput variable assignment and before the click() call
to open-calendar, ensuring the component is fully hydrated before attempting to
interact with it and avoiding potential timing flakes.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 1-20: The changeset file is missing the required issue-closing
reference syntax. Add a "Fixes #<issue-number>" line to the changeset summary
section (after the package and version type declaration and before or within the
description) to enable automatic GitHub issue closure when the changeset is
released. Replace <issue-number> with the actual GitHub issue number that this
changeset addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6635ad04-c22a-4c03-bb89-472d07f83ed1
📒 Files selected for processing (4)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
🛑 Comments failed to post (2)
packages/core/src/components/date-input/date-input.tsx (1)
795-804:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDropdown is closed on internal focus moves, which can break picker interactions.
Line 803 closes the dropdown for any non-null
relatedTarget. Onfocusout, that includes focus transitions within the component (input → picker/dropdown), so the picker can close mid-interaction.Suggested fix
- onFocusout={(e: FocusEvent) => { - const relatedTarget = e.relatedTarget as Node; - - // Related target might be null during rerenders, which would cause the dropdown to close unexpectedly - if (!relatedTarget) { - return; - } - - this.closeDropdown(); - }} + onFocusout={(e: FocusEvent) => { + const relatedTarget = e.relatedTarget as Node | null; + + // Related target might be null during rerenders + if (!relatedTarget) { + return; + } + + // Keep dropdown open while focus stays within this component/picker + if (this.hostElement.contains(relatedTarget)) { + return; + } + + this.closeDropdown(); + }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.onFocusout={(e: FocusEvent) => { const relatedTarget = e.relatedTarget as Node | null; // Related target might be null during rerenders if (!relatedTarget) { return; } // Keep dropdown open while focus stays within this component/picker if (this.hostElement.contains(relatedTarget)) { return; } this.closeDropdown(); }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 795 - 804, The onFocusout event handler is closing the dropdown whenever focus moves to any element outside the input field, including internal component elements like the picker. Instead of only checking if relatedTarget is not null, add an additional check to verify that the relatedTarget is outside the component itself by using contains() to check if this.el (the component's root element) contains the relatedTarget. Only call this.closeDropdown() when focus actually moves outside the component boundaries, not just when moving between internal child elements like the input and picker.packages/core/src/components/date-input/tests/date-input.ct.ts (1)
397-404:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd hydration assertion before interacting with the mounted component.
This test starts interactions immediately after
mount(...). Add the required hydration assertion first to avoid timing flakes.Suggested fix
await mount(`<ix-date-input required value=""></ix-date-input>`); const dateInput = page.locator('ix-date-input'); + await expect(dateInput).toHaveClass(/\bhydrated\b/); const input = dateInput.getByRole('textbox'); await dateInput.getByTestId('open-calendar').click();As per coding guidelines, "Always verify components are hydrated before making further assertions using
await expect(page.locator('ix-component')).toHaveClass(/\bhydrated\b/)."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await mount(`<ix-date-input required value=""></ix-date-input>`); const dateInput = page.locator('ix-date-input'); await expect(dateInput).toHaveClass(/\bhydrated\b/); const input = dateInput.getByRole('textbox'); await dateInput.getByTestId('open-calendar').click(); await expect(dateInput.getByTestId('date-dropdown')).toHaveClass(/show/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/tests/date-input.ct.ts` around lines 397 - 404, After the mount(...) call that creates the ix-date-input component, add a hydration assertion before any interactions occur. Insert an await expect statement that checks if the dateInput locator has the hydrated class using the regex pattern /\bhydrated\b/. This assertion should be placed immediately after the dateInput variable assignment and before the click() call to open-calendar, ensuring the component is fully hydrated before attempting to interact with it and avoiding potential timing flakes.Source: Coding guidelines
… or invalid - test cases added
…d streamline form submission tests
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
5-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
Fixes #<issue-number>to this changeset summary.The summary is missing the required issue-closing reference line. Please include the linked issue in
Fixes#1234`` format.As per coding guidelines, “Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/feat-date-input-validation.md around lines 5 - 21, The changeset summary in feat-date-input-validation.md is missing the required issue-closing reference line. Add a new line to the changeset summary that includes the `Fixes #<issue-number>` syntax (replacing issue-number with the actual GitHub issue number this changeset addresses) to ensure the linked issue is automatically closed when the changeset is released. This line should be included somewhere in the changeset content, typically at the beginning or end of the summary section.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/date-input/date-input.tsx`:
- Around line 796-805: The onFocusout handler is closing the dropdown whenever
relatedTarget is non-null, but this also triggers when focus moves to internal
elements within the date-input/picker component itself. Modify the condition to
check whether the relatedTarget is actually outside the component before calling
closeDropdown(). You should verify that the relatedTarget is not a descendant of
the date-input component (or the component itself) before closing the dropdown,
allowing internal focus transitions to proceed without interrupting
keyboard/calendar interactions.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 411-417: Add hydration verification before performing interactions
in the test blocks. After mounting the ix-date-input component, insert an
assertion to verify that the component has the hydrated class using the pattern
`await expect(page.locator('ix-date-input')).toHaveClass(/\bhydrated\b/)` before
calling any interaction methods like click() or type(). Apply this hydration
assertion to all the new test blocks mentioned in the additional ranges to
prevent racey test failures.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 5-21: The changeset summary in feat-date-input-validation.md is
missing the required issue-closing reference line. Add a new line to the
changeset summary that includes the `Fixes #<issue-number>` syntax (replacing
issue-number with the actual GitHub issue number this changeset addresses) to
ensure the linked issue is automatically closed when the changeset is released.
This line should be included somewhere in the changeset content, typically at
the beginning or end of the summary section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9579f5a-614a-4f9c-8939-558f93e8a8bb
📒 Files selected for processing (5)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/input/tests/form-ready.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/feat-date-input-validation.md (1)
5-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
Fixes #<issue-number>to this changeset summary.The summary is missing the required issue-closing reference line. Please include the linked issue in
Fixes#1234`` format.As per coding guidelines, “Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/feat-date-input-validation.md around lines 5 - 21, The changeset summary in feat-date-input-validation.md is missing the required issue-closing reference line. Add a new line to the changeset summary that includes the `Fixes #<issue-number>` syntax (replacing issue-number with the actual GitHub issue number this changeset addresses) to ensure the linked issue is automatically closed when the changeset is released. This line should be included somewhere in the changeset content, typically at the beginning or end of the summary section.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/date-input/date-input.tsx`:
- Around line 796-805: The onFocusout handler is closing the dropdown whenever
relatedTarget is non-null, but this also triggers when focus moves to internal
elements within the date-input/picker component itself. Modify the condition to
check whether the relatedTarget is actually outside the component before calling
closeDropdown(). You should verify that the relatedTarget is not a descendant of
the date-input component (or the component itself) before closing the dropdown,
allowing internal focus transitions to proceed without interrupting
keyboard/calendar interactions.
In `@packages/core/src/components/date-input/tests/date-input.ct.ts`:
- Around line 411-417: Add hydration verification before performing interactions
in the test blocks. After mounting the ix-date-input component, insert an
assertion to verify that the component has the hydrated class using the pattern
`await expect(page.locator('ix-date-input')).toHaveClass(/\bhydrated\b/)` before
calling any interaction methods like click() or type(). Apply this hydration
assertion to all the new test blocks mentioned in the additional ranges to
prevent racey test failures.
---
Outside diff comments:
In @.changeset/feat-date-input-validation.md:
- Around line 5-21: The changeset summary in feat-date-input-validation.md is
missing the required issue-closing reference line. Add a new line to the
changeset summary that includes the `Fixes #<issue-number>` syntax (replacing
issue-number with the actual GitHub issue number this changeset addresses) to
ensure the linked issue is automatically closed when the changeset is released.
This line should be included somewhere in the changeset content, typically at
the beginning or end of the summary section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9579f5a-614a-4f9c-8939-558f93e8a8bb
📒 Files selected for processing (5)
.changeset/feat-date-input-validation.mdpackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/input/tests/form-ready.ct.tspackages/core/src/components/utils/input/picker-input.util.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/utils/input/picker-input.util.ts
🛑 Comments failed to post (2)
packages/core/src/components/date-input/date-input.tsx (1)
796-805: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid closing the dropdown on internal focus moves.
Line 804 closes the dropdown for any non-null
relatedTarget. That also catches focus transitions inside the date-input/picker, which can interrupt keyboard/calendar interaction.Suggested fix
onFocusout={(e: FocusEvent) => { - const relatedTarget = e.relatedTarget as Node; + const relatedTarget = e.relatedTarget as Node | null; // Related target might be null during rerenders, which would cause the dropdown to close unexpectedly if (!relatedTarget) { return; } + const movedInsideHost = this.hostElement.contains(relatedTarget); + const movedInsidePicker = + this.dropdownElementRef.current?.contains(relatedTarget) ?? false; + + if (movedInsideHost || movedInsidePicker) { + return; + } + this.closeDropdown(); }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 796 - 805, The onFocusout handler is closing the dropdown whenever relatedTarget is non-null, but this also triggers when focus moves to internal elements within the date-input/picker component itself. Modify the condition to check whether the relatedTarget is actually outside the component before calling closeDropdown(). You should verify that the relatedTarget is not a descendant of the date-input component (or the component itself) before closing the dropdown, allowing internal focus transitions to proceed without interrupting keyboard/calendar interactions.packages/core/src/components/date-input/tests/date-input.ct.ts (1)
411-417: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add hydration assertions before interactions in the new tests.
These new tests click/type immediately after mount. Please assert hydration first to avoid racey failures.
Suggested pattern (apply to each new test block)
const dateInput = page.locator('ix-date-input'); + await expect(dateInput).toHaveClass(/\bhydrated\b/); await page.locator('button[type="submit"]').click();As per coding guidelines, “Always verify components are hydrated before making further assertions using
await expect(page.locator('ix-component')).toHaveClass(/\bhydrated\b/)”.Also applies to: 1027-1038, 1051-1063, 1076-1088
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/tests/date-input.ct.ts` around lines 411 - 417, Add hydration verification before performing interactions in the test blocks. After mounting the ix-date-input component, insert an assertion to verify that the component has the hydrated class using the pattern `await expect(page.locator('ix-date-input')).toHaveClass(/\bhydrated\b/)` before calling any interaction methods like click() or type(). Apply this hydration assertion to all the new test blocks mentioned in the additional ranges to prevent racey test failures.Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/date-input/date-input.tsx (1)
206-212: 📐 Maintainability & Code Quality | 🟠 MajorAdd Storybook stories for the new validation methods and error message customization.
The PR introduces user-visible APIs (
i18nErrorRequired,clear(),reportValidity()) with proper@since 5.1.0JSDoc tags and extensive component test coverage. However, the Storybook story atpackages/storybook-docs/src/stories/input-date.stories.tsonly demonstrates a basicrequiredcase and does not showcase these new validation features. Per path instructions, add story examples demonstrating the new methods and error message customization before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 206 - 212, Add new Storybook story examples to the `input-date.stories.ts` file that demonstrate the newly introduced validation features: the `i18nErrorRequired` property for customizing the error message, the `clear()` method for clearing the field, and the `reportValidity()` method for triggering validation. These stories should showcase realistic usage patterns and clearly illustrate how users can customize error messages and programmatically interact with validation on the date input component.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/components/date-input/date-input.tsx`:
- Around line 206-212: Add new Storybook story examples to the
`input-date.stories.ts` file that demonstrate the newly introduced validation
features: the `i18nErrorRequired` property for customizing the error message,
the `clear()` method for clearing the field, and the `reportValidity()` method
for triggering validation. These stories should showcase realistic usage
patterns and clearly illustrate how users can customize error messages and
programmatically interact with validation on the date input component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 739906df-09c2-4cc8-b3b4-449e86110054
📒 Files selected for processing (1)
packages/core/src/components/date-input/date-input.tsx
RamVinayMandal
changed the title
… Updated since tag
…for-date' of github.com:RamVinayMandal/ix into fix-ix-3322-display-input-as-valid-when-value-is-empty-for-date
…e-is-empty-for-date
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/date-input/date-input.tsx (1)
535-545: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the form-associated value in sync for invalid edits.
This branch updates
this.valueand validity state, but it skipsupdateFormInternalValue(...). After a touched field becomes invalid,ElementInternals.setFormValue()can still hold the previous valid date, soFormData(form)/manual form serialization can read stale data instead of the current control state.Suggested fix
if (this._hasInvalidInput) { this.invalidReason = validation.invalidReason; this.from = undefined; + this.updateFormInternalValue(value); } else { this.invalidReason = undefined; await syncRequiredValidationClass(this.hostElement, this); this.updateFormInternalValue(value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 535 - 545, The invalid-edit branch in date-input is leaving the form-associated value stale because it updates validity state without refreshing the internal form value. Update the invalid path in date-input.tsx so the same form sync logic used in the valid path also runs after setting invalidReason and clearing from; specifically, make sure updateFormInternalValue(...) is called from the branch around this._hasInvalidInput so ElementInternals.setFormValue() stays aligned with the current control state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/components/date-input/date-input.tsx`:
- Around line 535-545: The invalid-edit branch in date-input is leaving the
form-associated value stale because it updates validity state without refreshing
the internal form value. Update the invalid path in date-input.tsx so the same
form sync logic used in the valid path also runs after setting invalidReason and
clearing from; specifically, make sure updateFormInternalValue(...) is called
from the branch around this._hasInvalidInput so ElementInternals.setFormValue()
stays aligned with the current control state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d4b72a6-3dba-42d1-8d67-ee9407cf4f76
⛔ Files ignored due to path filters (2)
packages/angular/standalone/src/components.tsis excluded by!packages/angular/standalone/src/components.tspackages/react/src/components/components.server.tsis excluded by!packages/react/src/components/**
📒 Files selected for processing (5)
packages/angular/src/components.tspackages/core/src/components.d.tspackages/core/src/components/date-input/date-input.tsxpackages/core/src/components/date-input/tests/date-input.ct.tspackages/core/src/components/input/input.util.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/components/date-input/date-input.tsx (4)
140-144: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmit validity changes when
requiredchanges.Line 143 updates
ElementInternals, butvalidityStateChangelisteners are not notified when togglingrequiredchanges the field from valid to required-missing or back.Proposed fix
async onRequiredChange() { await syncRequiredValidationClass(this.hostElement, this); this.syncFormInternalsValidity(); + emitPickerValidityState(this); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 140 - 144, The onRequiredChange watcher in date-input.tsx updates ElementInternals validity but does not notify validityStateChange listeners when the required state toggles. After syncRequiredValidationClass and syncFormInternalsValidity in onRequiredChange, emit the validity change event from the same flow so consumers are notified whenever required switches the field between valid and required-missing states.Source: Path instructions
705-710: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not gate required validity on
touched.
touchedshould suppress visuals only. This makesgetValidityState()report required-empty fields as valid before interaction, whilesyncFormInternalsValidity()marks the same field invalid.Proposed fix
createValidityState( this.isInputInvalid, - !!this.required && this.touched, + !!this.required, this.value )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 705 - 710, The required-state logic in date-input.tsx is incorrectly gated by touched inside getValidityState/createValidityState, which makes required-empty values look valid until interaction. Update the validity calculation in the date-input component so required validation depends only on the required flag and current value, and keep touched limited to visual suppression elsewhere. Make the change in the getValidityState path and ensure it stays aligned with syncFormInternalsValidity for the DateInput component.Source: Path instructions
749-758: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSync internals inside
reportValidity().
reportValidity()updates_hasInvalidInputand visual classes, but leavesElementInternalsandinvalidReasonstale when the current invalidity comes from a recent programmatic value/constraint change.Proposed fix
`@Method`() async reportValidity(): Promise<boolean> { - const hasInvalidInput = - !!this.value && - !!this.format && - !this.getDateValidation(this.value).isValid; + const validation = + this.value && this.format ? this.getDateValidation(this.value) : undefined; + const hasInvalidInput = !!validation && !validation.isValid; this._hasInvalidInput = hasInvalidInput; + this.invalidReason = hasInvalidInput ? validation?.invalidReason : undefined; this._reportValidityCalled = true; + this.syncFormInternalsValidity(); return reportFieldValidity(this, hasInvalidInput); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 749 - 758, reportValidity() in date-input.tsx only refreshes the component’s local invalid state, so ElementInternals and invalidReason can stay out of sync after programmatic value or constraint changes. Update DateInput.reportValidity() to recompute the current validity state and synchronize the internals/invalidReason before calling reportFieldValidity(this, hasInvalidInput), using the existing validation helpers and the DateInput identifiers involved in validity tracking.Source: Path instructions
529-545: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the form-associated value in sync for invalid dates.
When validation fails,
updateFormInternalValue(value)is skipped, soFormDatacan keep the previous valid date whilevalueandvalueChangeexpose the new invalid string.Proposed fix
this._hasInvalidInput = !validation.isValid; this.isInputInvalid = this._hasInvalidInput && this.touched; + this.updateFormInternalValue(value); if (this._hasInvalidInput) { this.invalidReason = validation.invalidReason; this.from = undefined; } else { this.invalidReason = undefined; await syncRequiredValidationClass(this.hostElement, this); - this.updateFormInternalValue(value); this.closeDropdown(); focusInputIfKeyboardMode(this.inputElementRef.current); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/date-input/date-input.tsx` around lines 529 - 545, Keep the form-associated value synchronized even when date validation fails in handleValidatedInput. Right now the invalid branch in date-input.tsx updates invalidReason/from but skips updateFormInternalValue(value), so the component state and FormData can diverge; move or duplicate the form-value update so invalid input still writes the current raw value while preserving the existing invalid-state handling, and keep the successful path behavior in handleValidatedInput unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/components/date-input/date-input.tsx`:
- Around line 140-144: The onRequiredChange watcher in date-input.tsx updates
ElementInternals validity but does not notify validityStateChange listeners when
the required state toggles. After syncRequiredValidationClass and
syncFormInternalsValidity in onRequiredChange, emit the validity change event
from the same flow so consumers are notified whenever required switches the
field between valid and required-missing states.
- Around line 705-710: The required-state logic in date-input.tsx is incorrectly
gated by touched inside getValidityState/createValidityState, which makes
required-empty values look valid until interaction. Update the validity
calculation in the date-input component so required validation depends only on
the required flag and current value, and keep touched limited to visual
suppression elsewhere. Make the change in the getValidityState path and ensure
it stays aligned with syncFormInternalsValidity for the DateInput component.
- Around line 749-758: reportValidity() in date-input.tsx only refreshes the
component’s local invalid state, so ElementInternals and invalidReason can stay
out of sync after programmatic value or constraint changes. Update
DateInput.reportValidity() to recompute the current validity state and
synchronize the internals/invalidReason before calling reportFieldValidity(this,
hasInvalidInput), using the existing validation helpers and the DateInput
identifiers involved in validity tracking.
- Around line 529-545: Keep the form-associated value synchronized even when
date validation fails in handleValidatedInput. Right now the invalid branch in
date-input.tsx updates invalidReason/from but skips
updateFormInternalValue(value), so the component state and FormData can diverge;
move or duplicate the form-value update so invalid input still writes the
current raw value while preserving the existing invalid-state handling, and keep
the successful path behavior in handleValidatedInput unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a64fdbe2-ed13-4a37-a6cc-3b575f4aaf2d
📒 Files selected for processing (1)
packages/core/src/components/date-input/date-input.tsx
RamVinayMandal
changed the title
…for-date' of github.com:RamVinayMandal/ix into fix-ix-3322-display-input-as-valid-when-value-is-empty-for-date
…e-is-empty-for-date
|
2 participants
💡 What is the current behavior?
ix-date-inputshows as invalid when cleared, even though empty should be validix-date-inputdoesn't consistently show as invalid when the value is removednovalidateforms still display visual validation feedback despite opting out of validationrequiredattributeJIRA: IX-2595 IX-3322
🆕 What is the new behavior?
Validation Fixes
reportValidity()novalidateforms: Suppress all visual validation feedback;reportValidity()overrides this suppressionrequiredattribute: Validation state updates immediately when togglingNew APIs
clear()method to clear value and validation state (removes touched flag and error classes)reportValidity()method to trigger validation and show errors immediatelyi18nErrorRequiredprop to customize the required-field error messageValidation Behavior
novalidatesuppression is overridden byreportValidity()to show errorsAdditional Improvements
Note: The
clear(),reportValidity()methods andi18nErrorRequiredprop on ix-date-input is tagged with@since 5.2.0. Please verify this matches the actual target release version and we need to update the JSDoc tag if needed before merging.🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)Summary by CodeRabbit
New Features
clear()andreportValidity()toix-date-input, plusi18nErrorRequiredto customize required-field messaging.Bug Fixes
novalidateand form submission behavior to reflect current validity, including whenreportValidity()is used.Tests
reportValidity()UX/return values, calendar interaction, and form submit blocking) and accessibility coverage.